-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix compatibility issues with Python 2 #1004
Conversation
@@ -64,6 +64,7 @@ | |||
|
|||
''' | |||
|
|||
from __future__ import print_function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this future import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the end=' '
here:
print('Statistics after optimization:', end=' ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
@@ -1261,6 +1261,11 @@ | |||
" self.hp = ht_params\n", | |||
" \n", | |||
" def m_inf(self, D):\n", | |||
" # Workaround for handling complex numbers in Python 2\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I am not really happy with this work-around, it is a lot of distracting code for a notebook. And can D
really become negative? From the math, we should always have D>0
strictly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's not an elegant work-around. m_inf
is called from voltage_clamp
like this
m_old = channel.m_inf(DT_V_seq[0][1])
where channel
is iDK
and DT_V_seq
is the list in
nr, cr = voltage_clamp(iDK, [(500, -65.), (500, -35.), (500, -25.), (500, 0.), (5000, -70.)],
nest_dt=1.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess the problem is that in
def compute_I(self, t, V, m0, h0, D0):
self.D = si.odeint(self.dD, D0, t, args=(V,))
self.m = self.m_inf(self.D)
return - self.hp['g_peak_KNa'] * self.m * (V - self.hp['E_rev_KNa'])
self.D
may contain negative values (numerical error from odeint
). But we know that D
must always be strictly positive, so we should (i) check that we only get numerical noise below zero and then (ii) replace negative values in the result of odeint()
with the smallest possible positive number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstood my previous comment. voltage_clamp()
is called with a channel, and a list of interval-voltage pairs referred to as DT_V_seq
. In voltage_clamp
we have
# Control part
t_old = 0.
try:
m_old = channel.m_inf(DT_V_seq[0][1])
except NotImplementedError:
m_old = None
where DT_V_seq[0][1]
is the voltage of the first interval-voltage pair, i.e. -65.0
.
This is before it calls compute_I()
. And I checked the values for self.D
, they are all positive.
Corrected implementation in pure-python version of I_DK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve!
This PR contains some fixes for compatibility issues when using Python 2 when running examples and model_details scripts.
Python 2 doesn't handle complex numbers as elegantly as Python 3, which led to an error when running
HillTononiModels.ipynb
using Python 2. The problematic variable is now explicitly converted to a complex number if needed. There were also some minor issues in the PyNEST examples.